1911: perf: prune irrelevant partitions task - #79
Conversation
Co-authored-by: Andy Grove <agrove@apache.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Walkthrough
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces partition pruning for stage plans when the stage plan cache is disabled. It adds a helper function partition_prune_helper to prune partitions in ShuffleReaderExec nodes based on the required partition IDs before encoding the stage plan. The review feedback suggests optimizing partition_prune_helper by replacing the HashSet lookup with a direct linear scan (contains) on the partition_ids slice, which avoids heap allocation and is more efficient for small slices.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let wanted: HashSet<usize> = partition_ids.iter().copied().collect(); | ||
| Ok(plan | ||
| .clone() | ||
| .transform_up(|node| { | ||
| let Some(r) = node.as_any().downcast_ref::<ShuffleReaderExec>() else { | ||
| return Ok(Transformed::no(node)); | ||
| }; | ||
| if r.broadcast || r.partition.len() != n { | ||
| return Ok(Transformed::no(node)); | ||
| } | ||
|
|
||
| let partition = r | ||
| .partition | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, loc)| { | ||
| if wanted.contains(&i) { | ||
| loc.clone() | ||
| } else { | ||
| vec![] | ||
| } | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
Since partition_ids typically contains only a single partition ID (or a very small number of them in multi-task scenarios), allocating a HashSet and hashing the keys can be less efficient than a simple linear scan on the slice. Using partition_ids.contains(&i) directly avoids heap allocation and is faster for small slices.
Ok(plan
.clone()
.transform_up(|node| {
let Some(r) = node.as_any().downcast_ref::<ShuffleReaderExec>() else {
return Ok(Transformed::no(node));
};
if r.broadcast || r.partition.len() != n {
return Ok(Transformed::no(node));
}
let partition = r
.partition
.iter()
.enumerate()
.map(|(i, loc)| {
if partition_ids.contains(&i) {
loc.clone()
} else {
vec![]
}
})
.collect();
🤖 Augment PR SummarySummary: This PR reduces per-task plan payload size by pruning shuffle partition metadata that is irrelevant for the specific task partitions being launched. Changes:
Technical Notes: The pruning is implemented via a tree transform over the physical plan and currently only targets non-broadcast 🤖 Was this summary useful? React with 👍 or 👎 |
| partition_ids: &[usize], | ||
| plan: &Arc<dyn ExecutionPlan>, | ||
| ) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let n = plan.output_partitioning().partition_count(); |
There was a problem hiding this comment.
ballista/scheduler/src/state/task_manager.rs:174: partition_prune_helper can turn non-wanted ShuffleReaderExec.partition[i] entries into empty location lists, which will silently produce empty input if a non-included partition ever gets executed (or if an out-of-range partition_id is passed). Consider validating that partition_ids are in-range and correspond exactly to the partitions that will be executed with this encoded plan.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Code ReviewThis PR adds a Findings1. Prompt injection attempt —
|
1911: To review by AI